Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(OLD) Add cargo-auditable config option. #1516

Closed
wants to merge 6 commits into from
Closed

(OLD) Add cargo-auditable config option. #1516

wants to merge 6 commits into from

Conversation

duckinator
Copy link
Contributor

@duckinator duckinator commented Nov 1, 2024 •
edited
Loading

Moved to #1528 because the slash in the duckinator/auditable-builds branch name is incompatible with testing dist from a GitHub branch.

deferred:

complete:

  • use cargo auditable build instead of cargo build if cargo-auditable=true
  • install cargo-auditable in workflows if cargo-auditable=true
  • tests for both of those
  • real-world local test
  • real-world CI test

@ashleygwilliams ashleygwilliams added this to the 0.26.0 milestone Nov 1, 2024
@duckinator duckinator force-pushed the duckinator/auditable-builds branch from 61136ea to c684c8c Compare November 1, 2024 17:37
@duckinator
Copy link
Contributor Author

duckinator commented Nov 1, 2024 •
edited
Loading

image

@duckinator duckinator linked an issue Nov 1, 2024 that may be closed by this pull request
Build binaries with cargo auditable #81
Closed
@duckinator duckinator force-pushed the duckinator/auditable-builds branch from c684c8c to a501da3 Compare November 1, 2024 18:37
@duckinator
Copy link
Contributor Author

This is the current failure mode for auditable = true without cargo-auditable. I have mixed feelings about it — it's not terrible, but something like this is probably better:

Setting `auditable = true` requires `cargo auditable`. You can install it with `cargo install cargo-auditable`

image

@duckinator duckinator force-pushed the duckinator/auditable-builds branch from a501da3 to c0759ff Compare November 1, 2024 19:24
@duckinator duckinator changed the title [WIP] Add auditable config option. Add cargo-auditable config option. Nov 1, 2024
@duckinator duckinator marked this pull request as ready for review November 1, 2024 20:13
@mistydemeo
Copy link
Contributor

Since our main build environment is GitHub Actions at the moment, and it appears cargo auditable isn't preinstalled there, we should also make sure we handle installing it before doing local builds if it's in use. That'd be more impactful for our users than just telling them they need to install it. (We should also do that for non-CI builds though!)

@ashleygwilliams
Copy link
Member

cargo-auditable uses dist and so we can rely on their installers for our generated workflows and locally https://github.com/rust-secure-code/cargo-auditable/releases/tag/v0.6.4

@mistydemeo
Copy link
Contributor

I suppose then we should also consider: do we install a floating "whatever's latest" version? Or do we pick a known-good version and update it periodically?

@ashleygwilliams
Copy link
Member

@mistydemeo yeah it's a good question- i could see us going either way re pin latest or pick a known good release- both of those options beg the question of a config to override version which seems prudent, because we'll likely need it either way

@ashleygwilliams
Copy link
Member

@Shnatsel do you have thoughts on how we handle the version of cargo-auditable we install for folks?

@duckinator duckinator force-pushed the duckinator/auditable-builds branch 2 times, most recently from ea891cc to aa196ca Compare November 4, 2024 22:43
@duckinator duckinator force-pushed the duckinator/auditable-builds branch from aa196ca to e0d13f5 Compare November 4, 2024 22:47
mistydemeo
mistydemeo reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed checking for if there are no cargo workspaces - you can handle that by checking the DistGraphBuilder.workspaces property, which has a list of all workspaces. Each of those has a WorkspaceKind you can key off of. You can see us using that here in DistGraphBuilder::compute_build_steps:

cargo-dist/cargo-dist/src/tasks.rs

Lines 2435 to 2448 in d8ec1f9

fn compute_build_steps(&mut self) -> DistResult<()> {
// FIXME: more intelligently schedule these in a proper graph?
let mut local_build_steps = vec![];
let mut global_build_steps = vec![];
for workspace_idx in self.workspaces.all_workspace_indices() {
let workspace_kind = self.workspaces.workspace(workspace_idx).kind;
let builds = match workspace_kind {
axoproject::WorkspaceKind::Javascript => self.compute_generic_builds(workspace_idx),
axoproject::WorkspaceKind::Generic => self.compute_generic_builds(workspace_idx),
axoproject::WorkspaceKind::Rust => self.compute_cargo_builds(workspace_idx)?,
};
local_build_steps.extend(builds);

self.inner on DistGraphBuilder is DistGraph, so you've got access to the config there. It's probably good for us to handle this stuff early, so when we're computing the builds - or something similar early in the dist graph construction process - is probably a good place for us to check and potentially error out.

@Shnatsel
Copy link

Shnatsel commented Nov 5, 2024

@ashleygwilliams I treat the CLI as the public API for semver purposes, so installing the latest version in 0.6.x series is a safe bet. That should get you bug fixes without any kind of breakage (unless I mess up really badly).

You can revisit this if/when I ship 0.7.x, which right now is not even on the horizon.

This was referenced Nov 5, 2024
Closed
Open
@duckinator duckinator force-pushed the duckinator/auditable-builds branch from 8b8d193 to 0cb5667 Compare November 5, 2024 19:48
@duckinator duckinator force-pushed the duckinator/auditable-builds branch from edb9d42 to e4ade7f Compare November 5, 2024 19:57
@duckinator duckinator mentioned this pull request Nov 5, 2024
Merged
5 tasks
@duckinator
Copy link
Contributor Author

Moving to #1528 because of branch naming making it harder to test.

@duckinator duckinator closed this Nov 5, 2024
@duckinator duckinator changed the title Add cargo-auditable config option. (OLD) Add cargo-auditable config option. Nov 5, 2024
@duckinator duckinator deleted the duckinator/auditable-builds branch November 5, 2024 20:38
@ashleygwilliams ashleygwilliams removed this from the 0.26.0 milestone Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistydemeo mistydemeo mistydemeo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build binaries with cargo auditable
4 participants
@duckinator @mistydemeo @ashleygwilliams @Shnatsel